fix:CI#3199
Conversation
WalkthroughReduce test instance resource defaults, ensure reliable cleanup in scripts and CI (disk-space checks and broader artifact removal), add log-directory fallback and stale-log pruning in Codis admin scripts, and refine Codis/pika startup, orchestration, and resync in integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI runner / start script
participant FS as Filesystem (configs, data dirs)
participant Pika1 as pika:8000
participant Pika2 as pika:8001
participant Builder as Codis build (make)
participant Dash as Codis Dashboard
participant Proxy as Codis Proxy
participant FE as Codis Frontend
CI->>FS: prepare configs & data dirs (copy/sed reduced-memory)
CI->>Pika1: start pika:8000 (reduced memory/config)
CI-->>CI: wait for readiness
CI->>Pika2: start pika:8001 (reduced memory/config)
CI-->>CI: wait for readiness
CI->>Builder: build Codis (make)
Builder-->>CI: build complete
CI->>Dash: start dashboard (log-dir fallback & rotate)
CI->>Proxy: start proxy (log-dir fallback & rotate)
CI->>FE: start frontend (log-dir fallback & rotate)
CI->>Dash: create groups & set masters (env VARs)
CI->>Dash: assign slot ranges (0-511 -> gid1, 512-1023 -> gid2)
CI->>Dash: resync all groups
Dash->>Proxy: notify slot changes
Proxy->>Pika1: route/slot ops
Proxy->>Pika2: route/slot ops
CI->>CI: disk-space checks & cleanup (rm -f, prune)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pikatests.sh (1)
8-9: Trap-based cleanup now always clears logs/db on exit; confirm this is desired and consider simplifying calls
- Switching to
rm -f src/redis-serveris good: it avoids spurious errors if the binary is missing.- Adding
trap cleanup EXITmeanscleanupruns on every script exit (success or failure). Previously, after a successful run without$2 == "clean", logs and db directories were left around for inspection; now they are removed unconditionally.- Because of the trap, the explicit
cleanupcalls in the failure path (Lines 73–76) and the optional"clean"path (Lines 81–82) will cause cleanup to run twice, and the"clean"argument is effectively redundant.If the goal is CI-only usage, this is probably fine; for local debugging it may be useful to either:
- Gate the trap on an environment flag (e.g.
if [[ -n "$CI" ]]; then trap cleanup EXIT; fi), or- Keep the trap but drop the extra
cleanupcalls and/or repurpose$2 == "clean"to disable automatic cleanup.Also applies to: 12-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pikatests.sh(1 hunks)tests/conf/pika.conf(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: build_on_ubuntu
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
tests/conf/pika.conf (1)
61-61:expire-logs-nums : 3violates documented minimum of 10The resource reductions are directionally consistent for CI:
write-buffer-size : 64Mandmax-write-buffer-size : 1073741824significantly lower memtable pressure.target-file-size-base : 10M,binlog-file-size : 10485760,max-cache-files : 1000, andcache-maxmemory : 536870912all reduce disk and memory footprint, which should help avoid OOMs in constrained environments.However,
expire-logs-nums : 3is below the documented minimum of 10 and will likely be rejected or silently clamped by the Pika engine. Combined with the much smallerbinlog-file-size, this also gives extremely short binlog retention.Change to:
-expire-logs-nums : 3 +expire-logs-nums : 10Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/start_codis.sh (1)
34-35: CRITICAL: sed -i '' syntax is macOS-specific and will fail on Linux CI runners.The
sed -i '' -esyntax is BSD/macOS specific. GNU sed (used on ubuntu and centos CI runners) does not recognize the empty string argument and will fail with "unknown command" errors. This blocks the ubuntu and centos workflow jobs.Use portable sed syntax compatible with both GNU and BSD sed:
-sed -i '' -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8000|' -e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8000.conf +sed -i -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8000|' -e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8000.conf -sed -i '' -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8001|' -e 's|log-path : ./log/|log-path : ./codis_data_2/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_2/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_2/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_2/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_2/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8001.conf +sed -i -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8001|' -e 's|log-path : ./log/|log-path : ./codis_data_2/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_2/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_2/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_2/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_2/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8001.confAlternatively, if you need true in-place backup:
sed -i.bak -e '...' file # creates file.bak, works on both GNU and BSD
🧹 Nitpick comments (1)
.github/workflows/pika.yml (1)
71-80: Aggressive disk-space cleanup is effective but note removal scope.The new "Check Disk Space" steps include broad system-directory cleanup (
/usr/share/dotnet,/opt/ghc,/usr/local/share/boost). While effective for freeing space in resource-constrained CI, verify that no downstream steps or concurrent jobs depend on these paths. The use ofsudois appropriate in GitHub Actions, but confirm your runner configuration supports it.Also note: macOS job (build_on_macos) lacks a "Check Disk Space" step, creating observability inconsistency. Consider adding parity.
Also applies to: 137-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/pika.yml(5 hunks)codis/admin/codis-dashboard-admin.sh(1 hunks)codis/admin/codis-fe-admin.sh(1 hunks)codis/admin/codis-proxy-admin.sh(1 hunks)tests/integration/start_codis.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: build_on_ubuntu
🔇 Additional comments (5)
codis/admin/codis-dashboard-admin.sh (1)
22-33: Robust log-directory resilience and cleanup—approved.The graceful fallback to /tmp and log rotation via
find -mtime +1 -deleteare solid patterns for CI resilience. Note that while the /tmp fallback ensures script continuity, logs won't persist in the intended directory if creation fails; this trade-off is acceptable for admin scripts but may complicate debugging in production environments..github/workflows/pika.yml (1)
48-53: Consistent cleanup across jobs with improved naming and observability.The rename to "Cleanup Build Artifacts" and addition of
df -himprove clarity. Removed artifacts are appropriate (buildtrees, deps/src, CMakeFiles). Good hygiene overall.Also applies to: 122-127, 193-200
tests/integration/start_codis.sh (1)
3-17: Cleanup structure is sound and appropriately handles errors.The process termination, directory cleanup with
|| truefallback, and explicit recreation of codis directories ensure a clean test environment. Thedf -houtput aids in debugging disk-space issues. This pattern is solid.codis/admin/codis-fe-admin.sh (1)
24-35: Consistent with dashboard and proxy admin scripts—approved.The identical log-directory fallback and cleanup pattern across
codis-dashboard-admin.sh,codis-fe-admin.sh, andcodis-proxy-admin.shprovides uniform resilience and disk hygiene. While the duplication could benefit from a shared utility (e.g.,common-admin.sh), keeping these separate is acceptable given they are independent startup orchestrators.codis/admin/codis-proxy-admin.sh (1)
23-34: Mirrors dashboard and fe admin scripts—consistency achieved.The log-directory fallback and cleanup logic are now uniform across all three admin scripts. Approved.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/start_codis.sh (1)
25-28: Remove unused Pika config file copies.The script copies four Pika configuration files (lines 25–28), but only two instances are started (lines 37–38). The unused
pika_8002.confandpika_8003.conffiles waste disk space and create confusion.Remove the unused copies:
cp -f ../conf/pika.conf ./pika_8000.conf cp -f ../conf/pika.conf ./pika_8001.conf -cp -f ../conf/pika.conf ./pika_8002.conf -cp -f ../conf/pika.conf ./pika_8003.conf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/start_codis.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
| sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8000|' -e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8000.conf | ||
| sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8001|' -e 's|log-path : ./log/|log-path : ./codis_data_2/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_2/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_2/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_2/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_2/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8001.conf |
There was a problem hiding this comment.
Clean up sed backup files to prevent disk space waste.
The sed commands use -i.bak, which creates backup files (.bak extension) that persist on disk. Since the PR goal is to tighten resource usage and improve cleanup, these backups should be explicitly removed.
Add cleanup after the sed commands to remove backup files:
sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8000|' -e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8000.conf
sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8001|' -e 's|log-path : ./log/|log-path : ./codis_data_2/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_2/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_2/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_2/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_2/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8001.conf
+rm -f ./*.bakAlternatively, use -i without the backup suffix (if the base configuration files don't need to be preserved):
-sed -i.bak -e 's|databases : 1|databases : 2|' ... ./pika_8000.conf
+sed -i -e 's|databases : 1|databases : 2|' ... ./pika_8000.conf📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8000|' -e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8000.conf | |
| sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8001|' -e 's|log-path : ./log/|log-path : ./codis_data_2/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_2/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_2/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_2/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_2/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8001.conf | |
| sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8000|' -e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8000.conf | |
| sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8001|' -e 's|log-path : ./log/|log-path : ./codis_data_2/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_2/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_2/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_2/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_2/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8001.conf | |
| rm -f ./*.bak |
🤖 Prompt for AI Agents
In tests/integration/start_codis.sh around lines 34 to 35, the sed invocations
use -i.bak which leaves .bak backup files on disk; remove these residual files
by either switching to in-place editing without a suffix (use -i) if original
files don't need preserving, or immediately delete the created .bak files after
each sed call (e.g., rm ./pika_8000.conf.bak and rm ./pika_8001.conf.bak) to
ensure no backups persist.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/integration/start_codis.sh (2)
38-38: Fix incomplete template comment.The comment still contains template text
"Example Change the location..."which obscures the actual intent. Replace with a clear, concise description of what the sed commands do.-# Example Change the location for storing data on primary and secondary nodes in the configuration file +# Update pika configurations with data storage paths for primary and secondary nodes
39-44: Remove sed backup files to prevent disk space waste.The sed commands use
-i.bak, creating.bakbackup files that persist on disk. Since the PR goal is to tighten resource usage and cleanup, remove these backups. Either remove the backup suffix or delete the files afterward.sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8000|' -e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8000.conf sed -i.bak -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8001|' -e 's|log-path : ./log/|log-path : ./codis_data_2/log/|' -e 's|db-path : ./db/|db-path : ./codis_data_2/db/|' -e 's|dump-path : ./dump/|dump-path : ./codis_data_2/dump/|' -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_2/pika.pid|' -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_2/dbsync/|' -e 's|#daemonize : yes|daemonize : yes|' ./pika_8001.conf -sed -i.bak -e 's|#max-memory : 0|max-memory : 512|' ./pika_8000.conf -sed -i.bak -e 's|#max-memory : 0|max-memory : 512|' ./pika_8001.conf +sed -i -e 's|#max-memory : 0|max-memory : 512|' ./pika_8000.conf +sed -i -e 's|#max-memory : 0|max-memory : 512|' ./pika_8001.conf +rm -f ./*.bak
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/start_codis.sh(2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
tests/integration/start_codis.sh
[warning] 56-56: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_macos
- GitHub Check: build_on_centos
- GitHub Check: build_on_ubuntu
🔇 Additional comments (1)
tests/integration/start_codis.sh (1)
3-63: Overall approach is solid — cleanup and resource optimizations align well with PR objectives.The script improvements (enhanced cleanup, conditional compilation, memory limits, process monitoring) strengthen the CI reliability and resource efficiency goals. After addressing the three issues above (template comment, sed backups, cd error handling), this is ready.
| # Display memory usage after starting pika instances | ||
| free -h || true | ||
|
|
||
| cd ../codis |
There was a problem hiding this comment.
Add error handling to cd command.
If the directory change fails, subsequent commands will execute in the wrong location, causing silent failures. Add error handling to exit or return on failure.
-cd ../codis
+cd ../codis || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cd ../codis | |
| cd ../codis || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 56-56: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In tests/integration/start_codis.sh around line 56, the script runs "cd
../codis" without checking for failure; update the script to check the exit
status of the cd command and abort on failure (e.g., test the return value or
use "cd ../codis || { echo 'Failed to change directory to ../codis' >&2; exit 1;
}") so subsequent commands do not run in the wrong directory.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/integration/start_codis.sh (2)
78-78: Add error handling to thecdcommand.If the directory change fails, subsequent commands execute in the wrong location, silently breaking the build. Add error handling:
-cd ../codis +cd ../codis || exit 1
38-39: Remove the old template-like comment; keep only the new one.Lines 38 and 39 now contain redundant comments—the old "Example Change the location..." text on line 38 remains alongside the new explanatory comment on line 39. This duplication contradicts the prior PR feedback (commit 192207f). Remove line 38 entirely:
-# Example Change the location for storing data on primary and secondary nodes in the configuration file # Reduce memory usage for CI environment
🧹 Nitpick comments (1)
tests/integration/start_codis.sh (1)
82-86: Quote the command substitution to prevent word splitting.Line 84 has an unquoted command substitution. Quote it for robustness:
-if [ $(id -u) -eq 0 ]; then +if [ "$(id -u)" -eq 0 ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/start_codis.sh(2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
tests/integration/start_codis.sh
[warning] 78-78: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 84-84: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_on_macos
- GitHub Check: Analyze (go)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_centos
🔇 Additional comments (3)
tests/integration/start_codis.sh (3)
3-22: ✓ Solid cleanup and resource monitoring setup.The cleanup section effectively terminates processes, removes stale data directories, and resets codis artifacts while capturing memory and disk state for observability. The use of
|| trueguards prevents script abortion on expected edge cases.
68-77: ✓ Pika startup orchestration is sound.Sequential startup with staggered delays and memory monitoring provides good observability and resilience. No issues identified.
91-113: ✓ Codis orchestration is well-structured.The group creation, master assignment, slot range definition, and resync orchestration are comprehensive and well-coordinated. Use of environment variables for master addresses is clean and enables flexibility.
| sed -i.bak \ | ||
| -e 's|databases : 1|databases : 2|' \ | ||
| -e 's|port : 9221|port : 8000|' \ | ||
| -e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' \ | ||
| -e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' \ | ||
| -e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' \ | ||
| -e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' \ | ||
| -e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' \ | ||
| -e 's|#daemonize : yes|daemonize : yes|' \ | ||
| -e 's|cache-maxmemory : 10737418240|cache-maxmemory : 268435456|' \ | ||
| -e 's|max-write-buffer-size : 10737418240|max-write-buffer-size : 268435456|' \ | ||
| -e 's|write-buffer-size : 256M|write-buffer-size : 64M|' \ | ||
| ./pika_8000.conf |
There was a problem hiding this comment.
Clean up sed backup files to prevent CI disk waste.
Both sed commands use -i.bak, creating backup files (.bak) that persist on disk. This was previously flagged as a major resource-cleanup concern and contradicts the PR goal to tighten CI resource usage. Remove the backup files after each sed invocation:
sed -i.bak \
-e 's|databases : 1|databases : 2|' \
-e 's|port : 9221|port : 8000|' \
-e 's|log-path : ./log/|log-path : ./codis_data_1/log/|' \
-e 's|db-path : ./db/|db-path : ./codis_data_1/db/|' \
-e 's|dump-path : ./dump/|dump-path : ./codis_data_1/dump/|' \
-e 's|pidfile : ./pika.pid|pidfile : ./codis_data_1/pika.pid|' \
-e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_1/dbsync/|' \
-e 's|#daemonize : yes|daemonize : yes|' \
-e 's|cache-maxmemory : 10737418240|cache-maxmemory : 268435456|' \
-e 's|max-write-buffer-size : 10737418240|max-write-buffer-size : 268435456|' \
-e 's|write-buffer-size : 256M|write-buffer-size : 64M|' \
./pika_8000.conf
+rm -f ./pika_8000.conf.bak
sed -i.bak \
-e 's|databases : 1|databases : 2|' \
-e 's|port : 9221|port : 8001|' \
-e 's|log-path : ./log/|log-path : ./codis_data_2/log/|' \
-e 's|db-path : ./db/|db-path : ./codis_data_2/db/|' \
-e 's|dump-path : ./dump/|dump-path : ./codis_data_2/dump/|' \
-e 's|pidfile : ./pika.pid|pidfile : ./codis_data_2/pika.pid|' \
-e 's|db-sync-path : ./dbsync/|db-sync-path : ./codis_data_2/dbsync/|' \
-e 's|#daemonize : yes|daemonize : yes|' \
-e 's|cache-maxmemory : 10737418240|cache-maxmemory : 268435456|' \
-e 's|max-write-buffer-size : 10737418240|max-write-buffer-size : 268435456|' \
-e 's|write-buffer-size : 256M|write-buffer-size : 64M|' \
./pika_8001.conf
+rm -f ./pika_8001.conf.bakAlternatively, if the original config files do not need to be preserved, use sed -i without the backup suffix.
Also applies to: 54-66
🤖 Prompt for AI Agents
In tests/integration/start_codis.sh around lines 40 to 52 (and similarly for
lines 54 to 66), the sed invocations use -i.bak which leaves .bak backup files
on disk; change the sed calls to either use in-place editing without a backup
(sed -i) if preserving originals is unnecessary, or keep -i.bak but delete the
generated .bak files immediately after each sed command (e.g., rm -f
./pika_8000.conf.bak and other .bak files), ensuring no leftover .bak files
remain in CI to save disk space.
2 participants
Summary by CodeRabbit
Tests
Chores
CI / Automation
Maintenance
✏️ Tip: You can customize this high-level summary in your review settings.